-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add proposal for adding Keyring CMM and updating Default CMM definition #220
base: master
Are you sure you want to change the base?
Conversation
|
||
The specific CMM configuration describes a safe default that serves most use cases. | ||
|
||
The CMM configuration defined by the Default CMM is the [Keyring CMM](#keyring-cmm) as is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the Keyring CMM take an option for the default algorithm
and then the Default CMM defines this option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. If you are suggesting that we be able to initialize the KeyringCMM with a "default alg" option that overrides the default defined in the alg suite, then that would be changing the current behavior of the Default CMM and is out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was suggesting that the Keyring CMM takes an option for "default alg",
and then when the Keyring CMM is created in the Default CMM,
then the option would be what we define as a default.
So the ESDK default algorithm is defined in the Default CMM and not in the Keyring CMM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the keyring CMM just requires that the request identify the algorithm suite?
I'm loathe to squeeze opinions about algorithm suite into a CMM that SHOULD only care about wrangling keyrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what you're proposing now. This would mean that the Default CMM is no longer just a configuration. Unless we also want to move to the "default to the default alg CMM" logic out of the Keyring CMM and into a new CMM.
If we were to do something like this, I agree with Matt's approach, the KeyringCMM should just fail if it doesn't get an alg suite. I'm not sure if splitting up the logic this way is useful or not though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense, but the keyring CMM should be easy to use on its own.
If customers chose to use only the keyring CMM and inject that into the ESDK, it would not work in the default case.
Maybe that is a good thing though.
What I would like is that the default algorithm suite for the ESDK is defined in the Default CMM, not the keyring CMM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I would like is that the default algorithm suite for the ESDK is defined in the Default CMM, not the keyring CMM.
Agreed. If you're opting out of the default then you don't get any of the defaults. I think this is reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity, we do not want to have the Keyring CMM take a REQUIRED option "default alg"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be misunderstanding this conversation, but near the end of the doc, we say "This CMM MUST NOT offer any additional features beyond the composed CMM created above." To me, this means the default CMM MAY offer the same configuration options as the thing it is configuring. However, it MAY additionally support sane defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After offline discussion:
- It is important that the Default CMM MUST represent an expression of default values without accompanying logic
- This defines all defaults for encryption and decryption in one place, and additionally makes these defaults more extendable for any future code that may use CMMs but is not necessarily our current Encrypt/Decrypt client APIs.
- By ensuring this is a strict configuration, we ensure that each behavior is encapsulated in a composable CMM which can be reused by customers, making it easy to compose CMMs which opt out of certain default behaviors or reuse default behaviors within another configuration.
- This also makes updating the default behaviors easy, and able to be done in a way to preserves composability of our behaviors.
- As such, the default algorithm suite is defined by the Default CMM
- Customers MUST be able to get this default algorithm suite statically
- One more behavior of the KeyringCMM in this version of the spec is that it defaults to the default algorithm. This is not desired and this behavior should be encapsulated outside the Keyring CMM implementation.
- The Keyring CMM MUST require an algorithm suite
- We should add a new CMM implementation DefaultRequestValues (name TBD) that is solely responsible for setting default values in the CMM's materials requests, and then passes that request to an underlying CMM.
- For this proposal, this should ONLY be setting some default algorithm suite for the get encryption materials request.
- The Default CMM should thus represent the composition of this new DefaultRequestValues CMM and a Keyring CMM.
- We need to add examples for composing CMMs together.
I will update the doc with this motivation and behavior.
|
||
The specific CMM configuration describes a safe default that serves most use cases. | ||
|
||
The CMM configuration defined by the Default CMM is the [Keyring CMM](#keyring-cmm) as is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be misunderstanding this conversation, but near the end of the doc, we say "This CMM MUST NOT offer any additional features beyond the composed CMM created above." To me, this means the default CMM MAY offer the same configuration options as the thing it is configuring. However, it MAY additionally support sane defaults.
Co-authored-by: Alex Cioc <[email protected]>
I don't like the name "Set Unspecified Request Values CMM", but I could not think of any other name that didn't use "Default" in some way that could be interpreted wrongly. Still trying to brainstorm, but accepting recommendations! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a very quick top-level question. Will read more thoroughly assuming my suggestion isn't valid :)
Any desired CMM implementation that intends to use keyrings to ensure valid materials SHOULD | ||
compose with the Keyring CMM. | ||
|
||
### Set Unspecified Request Values CMM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have the Encrypt operation fill in the default when not provided as input, instead of it being the CMM's responsibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have considered this (and this is what Java/Python originally did, I believe), but we want this to be defined in the CMM for two reasons:
- The logic for everything the ESDK expresses as a "default" is encapsulated at the same level, in the Default CMM that we provide to the user. This is simple, and means that we have only one place in code where we define what "default" is.
- If this logic is in the CMM as opposed to the client level, then any other code that theoretically can be built on top of CMMs is able to grab the defaults, instead of needing to duplicate the "set some default for the algorithm suite" behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On thinking about this further, I'm not really convinced by either reason:
- "in the Encrypt operation" would be an equally valid single place to encapsulate the defaults
- Setting a default value doesn't seem to be difficult enough to worry about never duplicating IF there is significant code out there using CMMs outside the context of Encrypt/Decrypt
I agree the "Set Unspecified Request Values CMM" is valid and possible, but it feels like a really heavyweight solution. It would be a lot simpler to say that CMMs require the algorithm suite to be set in all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After offline discussion I'm at least convinced this is a reasonable solution - I'll make suggestions in the text for how we can justify it a bit more strongly.
Co-authored-by: Robin Salkeld <[email protected]>
How about "Default-Providing CMM"? |
Customers SHOULD either use the [Default CMM](#default-cmm) | ||
or compose a CMM using the AWS Encryption SDK's provided CMM implementations | ||
(including the Keyring CMM). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implies that customers SHOULD NOT implement their own CMM, which isn't what we want. Perhaps we should say that most customers should use one of these two options, but implementing their own is also possible, and if they do they will probably want to compose it with other provided CMM implementations.
Issue #, if available: Proposal for reframing the Default CMM
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.